Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JWKS parsing #146

Merged
merged 8 commits into from
Jul 1, 2021
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jun 29, 2021

builds on top of #112

closes #112

@Thalhammer
Copy link
Owner

Looks, good, but I have a couple of ideas/additions:

  • Liniting fails, (there is a clang-format file in the root that should work with most ide's)
  • None of the functions have error_code overloads. We moved the entire library to allow usage without (nonfatal) exception and I'd like to continue this.
  • I really dislike the way asymetric keys are passed to the library right now (4 strings for public,private key and a password for each one). I'd like to move to a class abstracting this into a single argument (along with convenience methods) eventually and imho this sounds like a good chance to do so.
  • Testing could be more extensive (take a look at the coverage report), but we can add this later once its feature stable.

@ghost
Copy link
Author

ghost commented Jun 29, 2021

Thanks for the feedback

  1. I'll fix the linting issues right away
  2. For the error_code overloads, I'm thinking of targetting the following functions - what are your thoughts?
jwks::get_jwk
jwk::get_algorithm
jwk::get_key_id
jwk::get_x5c
jwk::get_jwk_claim

I will add more unit tests and think of a way to abstract away the asymmetric-keys like you suggested

@Thalhammer
Copy link
Owner

I was actually thinking more about the parsing and not the accessors. I am not a computer rn, but you could take a look at the existing code (in particular the verifier) to check which methods have error_codes.

@ghost
Copy link
Author

ghost commented Jun 29, 2021

The parse_jwk and parse_jwks methods invoke the constructors of their respective classes. These methods throw an exception only when the json is malformed, or when there are no keys available. I'm assuming that this is similar to the decode method, which also throws an exception in case of invalid input.

However, if you feel this shouldn't throw an exception and instead use the error_code mechanism - i will make the change, just wanted to confirm if that's the intended behavior.

@Thalhammer
Copy link
Owner

I'm assuming that this is similar to the decode method, which also throws an exception in case of invalid input.

I stand corrected on my own library. You are right they do throw on invalid input.
That said it might be a wise idea to move forward to using error_codes there as well.
@prince-chrismc what do you think about this ?

We normally have both overloads, one that takes an error_code and only throws if something really bad happens and one that basically calls the error_code overload and throws if the error code is set.

But I think it might be a good idea to get it merged in some form at least and then iron out this stuff. Shouldn't be a problem as long as it is not part of a release.

@ghost
Copy link
Author

ghost commented Jun 29, 2021

I agree - I can send follow-up PR's to change the parse_jwks, parse_jwk, and decode to take in an error_code. I've added a few more unit tests - the classes jwks and jwk are more or less completely covered now.

@Thalhammer
Copy link
Owner

Thalhammer commented Jun 30, 2021

Ok so I finally came around to actually download the branch and take a look at the jwk specification.
I noticed you only expose a selection of the valid properties (alg, kid and x5c), while

  • kty
  • use
  • key_ops
  • x5u
  • x5t
  • x5t#S256

dont have an accessor. Was this a deliberate decision (e.g. to keep the interface simpler) or an oversight ?
Depending on the kty there can also be other fields like x & y in elliptic curve keys (I don't think they should have direct accessors directly inside the jwk, rather the algorithm should probably get a constructor or static function to load a key from jwk).

It might also be a good idea to check that members required by the spec (e.g. kty) are present and have the correct format.

I don't think it makes sense to add parsing of the keys right now, instead I think we should merge this with the basic structure (which should already be helpfull to people) and follow up with a number of other pr's.
In particular I'd like to do the following:

  • Get the base stuff error_code ready and finish transition to error_code interfaces in normal jwt parsing
  • Add some sort of "key" type as a member of the algorithm (since they differ vastly) which has functions for parsing various formats (where they make sense), as well as parsing from jwk.
  • Add some sort of keychain class which is a list of keys grouped by algorithm (and keyid?)
  • Some way to add all keys in a jwk to the allowed algorithms in a verifier.

I am still wondering whether or not to take a look at encryption. Since we already depend on openssl/libressl we would not introduce new dependencies by adding it, but I wonder how many people actually use it (i.e. if its worth the effort).

@ghost
Copy link
Author

ghost commented Jun 30, 2021

It was an oversight - I had mistakenly assumed the OP had completely implemented the spec, my bad - I should have verified my assumptions.

I agree with your list of TODO items - and I think the next step after merging this is to transition to error_code interfaces and stabilize the API before adding more features

Copy link
Collaborator

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right they do throw on invalid input.
That said it might be a wise idea to move forward to using error_codes there as well.
@prince-chrismc what do you think about this ?

No objections! 👍

include/jwt-cpp/jwt.h Outdated Show resolved Hide resolved
include/jwt-cpp/jwt.h Show resolved Hide resolved
Comment on lines 3111 to 3114
* Create a verifier using the given clock
* \param c Clock instance to use
* \return verifier instance
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks out of place 🤔

include/jwt-cpp/jwt.h Outdated Show resolved Hide resolved
include/jwt-cpp/jwt.h Outdated Show resolved Hide resolved
@Thalhammer Thalhammer merged commit ae8d1ef into Thalhammer:master Jul 1, 2021
@Thalhammer
Copy link
Owner

@tchinmai7 Thanks for the work you put into this :)

@ghost
Copy link
Author

ghost commented Jul 1, 2021

You're welcome, and Thank you very much for this awesome library!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants